[CTL] Add support for defaults #1216

KFilipek · 2025-03-21T15:47:08Z

[CTL] Add support for defaults

Description

This PR introduces initial support for setting up default values.

Checklist

Code compiles without errors locally
All tests pass locally
CI workflows execute properly

src/ctl/ctl.c

src/memory_pool.c

src/pool/pool_disjoint_ctl.h

lukaszstolarczuk · 2025-03-24T15:42:37Z

test/pools/disjoint_pool_ctl.cpp

+    umf_os_memory_provider_params_handle_t os_memory_provider_params = NULL;
+    umf_memory_provider_ops_t *os_provider_ops = umfOsMemoryProviderOps();
+    if (os_provider_ops == NULL) {
+        GTEST_SKIP() << "OS memory provider is not supported!";


Instead of adding such skip msg, I guess we should add this test in CMake only when OS prov and disjoint pool are enabled.

It's what for we've added a dummy function.

yy? not sure what you mean

lukaszstolarczuk · 2025-03-24T15:49:14Z

test/ctl/ctl_api.cpp

@@ -35,7 +35,7 @@ TEST_F(test, ctl_by_handle_os_provider) {

    int ipc_enabled = 0xBAD;
    ret = umfCtlGet("umf.provider.by_handle.params.ipc_enabled", hProvider,


do we have any test for umfCtlGet that pass something else than 0?

As I see it's not a part of this PR

You're adding a new paramter and without testing values other than 0 it's not properly tested. At least mark in tests a TODO for future.

This callback discard completely size, so such test will cover completely nothing and will test nothing.

static int CTL_READ_HANDLER(ipc_enabled)(void *ctx, umf_ctl_query_source_t source, void *arg, size_t size, umf_ctl_index_utlist_t *indexes, const char *extra_name, umf_ctl_query_type_t query_type) { /* suppress unused-parameter errors */ (void)source, (void)indexes, (void)ctx, (void)extra_name, (void)query_type, (void)size; int *arg_out = arg; os_memory_provider_t *os_provider = (os_memory_provider_t *)ctx; *arg_out = os_provider->IPC_enabled; return 0; }

I'm reading your last comment as "size is useless" ;)

Let me re-phrase - whenever size is used, please add a test with various values (user may give some input we may not expect, e.g. on wrong assumptions on how this func works) or mark a TODO for adding tests in the future.

src/memory_pool.c

include/umf/memory_pool_ops.h

src/memory_pool.c

src/pool/pool_disjoint_ctl.h

test/ctl/ctl_api.cpp

test/pools/disjoint_pool.cpp

test/pools/disjoint_pool_ctl.cpp

include/umf/memory_pool_ops.h

include/umf/base.h

test/pools/disjoint_pool_ctl.cpp

lplewa

umf_result_t umfCtlExec(const char *name, void *ctx, void *arg); should also have arg size imho.

lplewa · 2025-03-25T15:21:07Z

src/ctl/ctl.c

@@ -416,11 +402,14 @@ int ctl_query(struct ctl *ctl, void *ctx, umf_ctl_query_source_t source,
        errno = EINVAL;
        goto out;
    }
+    const char *extra_name = &name[0];
+    if (strstr(extra_name, "by_handle") != NULL) {
+        extra_name = &name[0] + name_offset;


Please do not include UMF specyfic soultions in ctl* files.

lplewa · 2025-03-25T15:23:08Z

src/memory_pool.c

+                                        const char *extra_name,
+                                        umf_ctl_query_type_t queryType) {
+    (void)indexes, (void)source, (void)size, (void)ctx;
+    assert(queryType == CTL_QUERY_WRITE && "not supported query type");


This is not an assert. This sould return a correct error code to the user.

read should work

lplewa · 2025-03-25T15:30:07Z

src/memory_pool.c

@@ -99,6 +135,15 @@ static umf_result_t umfPoolCreateInternal(const umf_memory_pool_ops_t *ops,
        goto err_pool_init;
    }

+    // Set default property "name" to pool if exists
+    for (int i = 0; i < UMF_DEFAULT_SIZE; i++) {
+        if (CTL_DEFAULT_ENTRIES[i][0] != '\0') {


You should only call it for entries with matching name, not all.

Now, should be fine

lplewa · 2025-03-25T15:33:39Z

src/pool/pool_disjoint_ctl.h

@@ -0,0 +1,61 @@
+/*
+ * Copyright (C) 2025 Intel Corporation


imho this should not be a .h files

lukaszstolarczuk · 2025-03-26T09:48:27Z

test/ctl/ctl_api.cpp

@@ -35,7 +35,7 @@ TEST_F(test, ctl_by_handle_os_provider) {

    int ipc_enabled = 0xBAD;
    ret = umfCtlGet("umf.provider.by_handle.params.ipc_enabled", hProvider,


You're adding a new paramter and without testing values other than 0 it's not properly tested. At least mark in tests a TODO for future.

lukaszstolarczuk · 2025-03-26T09:49:31Z

test/pools/disjoint_pool_ctl.cpp

+    umf_os_memory_provider_params_handle_t os_memory_provider_params = NULL;
+    umf_memory_provider_ops_t *os_provider_ops = umfOsMemoryProviderOps();
+    if (os_provider_ops == NULL) {
+        GTEST_SKIP() << "OS memory provider is not supported!";


yy? not sure what you mean

test/ctl/ctl_api.cpp

include/umf/base.h

KFilipek added the enhancement New feature or request label Mar 21, 2025

KFilipek self-assigned this Mar 21, 2025

KFilipek linked an issue Mar 21, 2025 that may be closed by this pull request

CTL - setting global namespace #1058

Open

KFilipek force-pushed the ctl-default branch 5 times, most recently from 2ec6659 to 408f2c4 Compare March 24, 2025 08:16

KFilipek marked this pull request as ready for review March 24, 2025 09:43

KFilipek requested a review from a team as a code owner March 24, 2025 09:43

KFilipek force-pushed the ctl-default branch from 408f2c4 to 13bdb89 Compare March 24, 2025 15:35

lukaszstolarczuk reviewed Mar 24, 2025

View reviewed changes

bratpiorka requested changes Mar 24, 2025

View reviewed changes

bratpiorka reviewed Mar 25, 2025

View reviewed changes

include/umf/memory_pool_ops.h Outdated Show resolved Hide resolved

bratpiorka reviewed Mar 25, 2025

View reviewed changes

include/umf/base.h Outdated Show resolved Hide resolved

bratpiorka reviewed Mar 25, 2025

View reviewed changes

test/pools/disjoint_pool_ctl.cpp Outdated Show resolved Hide resolved

KFilipek force-pushed the ctl-default branch 4 times, most recently from b7324ed to 8e3a2fc Compare March 25, 2025 13:00

lplewa requested changes Mar 25, 2025

View reviewed changes

KFilipek force-pushed the ctl-default branch 2 times, most recently from 338947a to 29380ba Compare March 25, 2025 16:02

KFilipek requested review from lukaszstolarczuk and bratpiorka March 25, 2025 16:02

lukaszstolarczuk reviewed Mar 26, 2025

View reviewed changes

KFilipek force-pushed the ctl-default branch 4 times, most recently from a8a7fe8 to 56b5408 Compare March 26, 2025 11:57

KFilipek force-pushed the ctl-default branch 11 times, most recently from 312ed18 to db7eca2 Compare April 11, 2025 13:37

KFilipek force-pushed the ctl-default branch 3 times, most recently from f55ddb1 to 141d7bc Compare April 15, 2025 11:54

Add function to get pool name

136dc39

KFilipek force-pushed the ctl-default branch 9 times, most recently from 33ceeeb to 77692e1 Compare April 17, 2025 09:59

KFilipek requested a review from lplewa April 17, 2025 09:59

KFilipek force-pushed the ctl-default branch 3 times, most recently from 6a2ed97 to 0c02aaa Compare April 17, 2025 19:59

[CTL] Add support for defaults

f7c7900

KFilipek force-pushed the ctl-default branch from 0c02aaa to f7c7900 Compare April 20, 2025 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CTL] Add support for defaults #1216

[CTL] Add support for defaults #1216

KFilipek commented Mar 21, 2025 •

edited

Loading

lukaszstolarczuk Mar 24, 2025

KFilipek Mar 25, 2025

lukaszstolarczuk Mar 26, 2025

lukaszstolarczuk Mar 24, 2025

KFilipek Mar 25, 2025

lukaszstolarczuk Mar 26, 2025

KFilipek Mar 26, 2025 •

edited

Loading

lukaszstolarczuk Mar 27, 2025

lplewa left a comment

lplewa Mar 25, 2025

KFilipek Mar 25, 2025

lplewa Mar 25, 2025

lplewa Mar 25, 2025

KFilipek Mar 25, 2025

lplewa Mar 25, 2025

KFilipek Mar 25, 2025

lplewa Mar 25, 2025

KFilipek Mar 26, 2025

lukaszstolarczuk Mar 26, 2025

lukaszstolarczuk Mar 26, 2025

		@@ -35,7 +35,7 @@ TEST_F(test, ctl_by_handle_os_provider) {

		int ipc_enabled = 0xBAD;
		ret = umfCtlGet("umf.provider.by_handle.params.ipc_enabled", hProvider,

[CTL] Add support for defaults #1216

Are you sure you want to change the base?

[CTL] Add support for defaults #1216

Conversation

KFilipek commented Mar 21, 2025 • edited Loading

[CTL] Add support for defaults

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KFilipek Mar 26, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lplewa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KFilipek commented Mar 21, 2025 •

edited

Loading

KFilipek Mar 26, 2025 •

edited

Loading